Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Esc handling #220

Closed
wants to merge 9 commits into from
Closed

Esc handling #220

wants to merge 9 commits into from

Conversation

Olzaq
Copy link
Contributor

@Olzaq Olzaq commented Oct 25, 2016

ESC binding is disabled and instead it is used for current key cleanup. Fixes https://bugs.arx-libertatis.org/arx/issues/408

Copy link
Member

@dscharrer dscharrer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, I like the Config::setActionKey() cleanup.

@@ -344,7 +344,7 @@ void Config::setActionKey(ControlAction actionId, size_t index, InputKeyId key)
return;
}

// remove double key assignments
// remove existing key assignments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be merged with the previous commit.

@@ -344,6 +344,9 @@ void Config::setActionKey(ControlAction actionId, size_t index, InputKeyId key)
return;
}

if (key == Keyboard::Key_Escape)
Copy link
Member

@dscharrer dscharrer Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the space between "if" and "(".

Also, I'm not sure if it makes sense to special-case Escape in the Config class - we want to use Esc to clear key bindings, and that distinction should imo be higher up in the code.

Widget * widget = m_children.GetZoneWithID(BUTTON_MENUOPTIONS_CONTROLS_CUST_DEFAULT);

if(widget == m_selected) {
if(m_selected->GetZoneWithID(BUTTON_MENUOPTIONS_CONTROLS_CUST_DEFAULT)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your new code will match any widgets containing the "Default" button. Not sure if these can ever be "selected" though but it would be safer to just check the id.

Copy link
Contributor Author

@Olzaq Olzaq Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true though "Default" is not used by any other widgets than those 2 control configs.

I was thinking some more refactoring here would be good anyway as this part of MenuPage class contains lot of code which is used only by those 2 classes. And MenuPage is inherited by 13 classes. E.g. if we could move some controls related code to ControlOptionsPage? Any thoughts about that?

Doing cleanup before assingment makes post cleanup redundant.
@Eli2
Copy link
Member

Eli2 commented Dec 1, 2016

merged

@Eli2 Eli2 closed this Dec 1, 2016
@Olzaq Olzaq deleted the esc_handling branch December 1, 2016 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants